Update nodeBindings version #1533
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| sdkNodeModulesXmtpDir, | ||
| bindingsDir, | ||
| ); | ||
| fs.symlinkSync(relativeBindingsPath, symlinkTarget); |
There was a problem hiding this comment.
On Windows, fs.symlinkSync() to a directory without a type can fail or create the wrong link. Consider passing type: 'junction' on Windows (and 'dir' elsewhere) when linking node-bindings.
- fs.symlinkSync(relativeBindingsPath, symlinkTarget);
+ fs.symlinkSync(
+ relativeBindingsPath,
+ symlinkTarget,
+ process.platform === "win32" ? "junction" : "dir"
+ );🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unify SDK version management under
@helpers/versions, run bothyarn versionsandyarn agent-versionsviacli/versions.ts, and bump node-bindings used with node-sdk 4.3.0 to 1.6.1Introduce a consolidated versions module and CLI that centralize Agent SDK and Node SDK version resolution and symlink setup. Most imports move to
@helpers/versions, the legacyagents/versions/*andcli/sdk-versions.tsare removed, and the node-bindings alias updates to 1.6.1-rc2 for node-sdk 4.3.0.📍Where to Start
Start with the consolidated versions module in
helpers/versions.ts, then review the new combined CLI incli/versions.tsthat drives the updated scripts.📊 Macroscope summarized f014874. 24 files reviewed, 41 issues evaluated, 26 issues filtered, 1 comment posted
🗂️ Filtered Issues
agents/bots/key-check/handlers/forks.ts — 0 comments posted, 7 evaluated, 6 filtered
calculateTimeSinceLastMessageassumes the most recent message is at index0(const lastMessage = messages[0];). Ifconversation.messages()returns messages in chronological ascending order (oldest first), this produces a time since the oldest message instead of the latest, yielding incorrect negative/very large values and misleading reports. [ Low confidence ]calculateTimeSinceLastMessagedereferenceslastMessage.sentAt.getTime()without validating thatsentAtis aDate. Sincemessagesis cast toany[]and there is no schema validation, ifsentAtis missing or not aDate, this throws. Although this call sits within thetryblock for message analysis and is caught, it collapses the entire message analysis step and leads to misleadingsyncErrorsand missing metrics. [ Low confidence ]analyzeForkStateretrievesepochandmaybeForkedby callingconversation.debugInfo()twice for pre-sync and twice for post-sync (lines 63–66and76–79). This can produce inconsistent snapshots ifdebugInfo()values change between calls, leading to mismatchedepochandmaybeForkedin a singleForkDebugInfo. It can cause false-positive/false-negative fork detection and misleading logs. [ Low confidence ]buildForkReportformats timestamps viaformatTimestamponmessage.sentAt as Dateandconversation.createdAt as Datewithout verifying their types. If either field is missing or not aDate,toISOString()will throw, causing the report build to fail and the handler to enter the catch path. [ Already posted ]minutesAgo, the code does not guard against negative values (e.g., if the last message has a futuresentAtdue to clock skew). It will render"-5 minutes ago"in the report, which is misleading and violates basic output validity expectations. [ Low confidence ]handleForkDetection, the fallback sendawait ctx.sendText(errorMessage)is not wrapped in its own try/catch. If sending the error message fails (e.g., network/transient issue), the handler will reject without logging or alternative fallback, violating the guarantee that each input reaches a defined terminal state with visible outcome. [ Low confidence ]agents/bots/key-check/handlers/groups.ts — 0 comments posted, 7 evaluated, 7 filtered
handleGroupMembersat line 42,handleGroupAdminsat line 146, and their catch blocks at lines 49 and 159), inbox abbreviations are constructed usingmember.inboxId.substring(0, 8)andmember.inboxId.substring(member.inboxId.length - 8). If theinboxIdis shorter than 8 characters,member.inboxId.length - 8is negative; whilesubstringcoerces negative indices to 0, the resulting output will be malformed (e.g., duplicating the head or showing unexpected format). Add a safe abbreviation helper that checks length and formats edge cases without producing misleading output. [ Out of scope ]handleGroupMembers, the catch block unconditionally dereferencesmember.inboxIdto build the abbreviated inbox string:member.inboxId.substring(0, 8)...${member.inboxId.substring(member.inboxId.length - 8)}. If the original error was due tomemberbeingnull/undefinedormember.inboxIdbeing missing/undefined, this catch block will throw a second exception, aborting the entire handler and falling to the outer catch, instead of continuing to process other members. Add guards before usingmember.inboxId, or fall back to a safe placeholder string whenmemberormember.inboxIdis not available. [ Out of scope ]handleGroupInfo,groupIdis taken fromctx.message.conversationIdand immediately used with.substring(...)to render both abbreviated and full IDs. Ifctx.messageorconversationIdis missing or not a string, callingsubstringwill throw and abort the handler, leading to the outer catch path. Add a guard to ensuregroupIdis a non-empty string before using.substring, and provide a fallback output when unavailable. [ Out of scope ]handleGroupAdmins, the catch block unconditionally formatsmember.inboxIdusing.substring(...). If the original error was triggered by anull/undefinedmemberor missinginboxId, this will throw a second exception and abort the handler execution instead of continuing. Guardmemberandmember.inboxIdbefore formatting, or produce a safe placeholder when unavailable. [ Out of scope ]handleGroupPermissionsdoes not validatemembersbefore using it. It callsawait ctx.conversation.members()and then immediately usesmembers.find(...)andmembers.length. Ifmembersisnull/undefined(or not an array), these operations will throw and abort the handler. Add an explicit guard likeif (!members || !Array.isArray(members)) { ... }and handle the empty case consistently (similar tohandleGroupMembers). [ Out of scope ]handleGroupPermissionsperforms per-member address resolution without local error containment:member.accountIdentifiers.find(...)is called directly. Ifmemberexists butaccountIdentifiersis missing/non-array or containsundefinedelements, this can throw and abort the entire handler due to the top-level try/catch, producing a failure message instead of partially rendering valid entries. Wrap the per-member resolution in a try/catch or add guards to safely skip/mark unknown addresses when data is missing. [ Out of scope ]handleGroupPermissions, the 'Regular Members' count is computed asmembers.length - admins.length - superAdmins.length. Ifgroup.adminsorgroup.superAdminsinclude inbox IDs that are not present inmembers, this subtraction can produce a negative number, resulting in misleading or invalid output. To ensure a correct count, subtract only those admin/super-admin inbox IDs that are present in thememberslist. [ Out of scope ]agents/bots/key-check/handlers/ux.ts — 0 comments posted, 4 evaluated, 3 filtered
handleTransactionlacks error handling. Unlike the other handlers, it executes multiple async operations (ctx.getClientAddress(),ctx.getSenderAddress(), andctx.conversation.send(...)) without atry/catch. Any failure (e.g., inability to resolve addresses, messaging send failure) will result in an unhandled rejection that can crash the handler or propagate upstream without a defined terminal state. [ Out of scope ]handleTransactionusesgetClientAddress()andgetSenderAddress()outputs without validating that they are present and well-formed0x-prefixed addresses. If either address is empty or malformed,createUSDCTransferCallswill produce invalid transaction data byslice(2)and padding operations, resulting in malformed ABI-encodeddatafor the transfer call and a send that may fail at the wallet/client without a clear error path here. [ Low confidence ]handleDeeplinkconstructs a deeplink withagentAddress = ctx.client.accountIdentifier?.identifier || ""and proceeds even when it is empty, producingcbwallet://messaging/which is malformed and may mislead users or fail in the target client. There is no fallback or explicit error when the identifier is missing. [ Already posted ]cli/versions.ts — 1 comment posted, 9 evaluated, 8 filtered
path.resolve(sdkNodeModulesXmtpDir, currentTarget)withpath.resolve(sdkNodeModulesXmtpDir, expectedRelativePath)using strict string equality. On Windows’ case-insensitive filesystems, the same target with different casing could be treated as different by this comparison, causing unnecessary unlink/relink cycles. Consider normalizing case on Windows before comparison. [ Low confidence ]node_modulesdirectory on failure. In the cleanup block, if any error occurs while removingsdkNodeModulesXmtpDirorsymlinkTarget, the code catches and then executes: [ Low confidence ]fs.mkdirSync(sdkNodeModulesXmtpDir, { recursive: true })being outside any try/catch. If directory creation fails (permissions, concurrent removal, read-only FS), the function throws and aborts without settinghasErrorsor emitting a controlled error message. This is inconsistent with the error handling approach used for symlink creation and could leave the process in an ungraceful state. [ Low confidence ]process.exit(1)from within a utility that manipulates the filesystem. While used from a CLI, this hard exit prevents callers from handling errors gracefully or performing additional cleanup. IfcreateBindingsSymlinks()is ever reused in a programmatic context, it will terminate the whole process on partial failures. Consider returning a status or throwing, and let the CLI entrypoint decide to exit. [ Code style ]createAgentSDKSymlinksdeletes the existingnode_modules/@xmtp/node-sdkpath withfs.rmSync(..., { recursive: true, force: true })when it is not a symbolic link. If a real directory exists there (e.g., a previously installed package or nested dependency), this will silently delete its contents, potentially breaking the agent package’s local dependencies. This is a high-risk behavior when transitioning from a real directory to a symlink. [ Low confidence ]createAgentSDKSymlinksaggressively deletes the entirenode_modulesdirectory within each agent SDK package on any failure during symlink removal. In the catch block starting at line 254, if removal of the existing symlink/target fails, the code executesfs.rmSync(path.join(agentSDKDir, "node_modules"), { recursive: true, force: true }), which can irreversibly remove all dependencies of that package. This is highly destructive and can break the installation. Errors like permission issues, EBUSY, or transient FS problems would trigger this fallback. [ Low confidence ]fs.symlinkSyncis called without specifying the symlink type for a directory target. On Windows, creating directory symlinks often requires specifying the type"junction"(or"dir") to avoid failures or incorrect symlink kinds. Callingfs.symlinkSync(relativeNodeSDKPath, symlinkTarget)may fail or create an invalid link on Windows environments, causing setup to break even when paths are correct. [ Low confidence ]main(), when--cleanis passed andGITHUB_ACTIONSis not set, the code removes the entirenode_modulesdirectory and then attemptsexecSync("yarn install"). If that install fails, the code only logs a warning and "Continuing with existing installation..." but does not abort or restore the previous state. The next calls tocreateBindingsSymlinks()andcreateAgentSDKSymlinks()requirenode_modules/@xmtpto exist and willprocess.exit(1)if it does not, yielding an abrupt termination after irreversibly deletingnode_modules. This results in an inconsistent and misleading "continue" path that guarantees failure and leaves the workspace without dependencies. The failure path should either (a) abort early after the failed install, or (b) avoid deletingnode_modulesuntil after a successful reinstall, or (c) handle the missing@xmtpdirectories gracefully without terminating the process. [ Already posted ]helpers/versions.ts — 0 comments posted, 2 evaluated, 2 filtered
getActiveAgentVersioncan returnundefinedwhen the providedindexis out of bounds ofgetAgentVersions(). The function initializeslatestVersionwithversions[index]and, ifAGENT_SDK_VERSIONis not set, it returns this value without validation. Callers expecting a valid version object may crash or behave incorrectly whenundefinedis returned. [ Low confidence ]checkAgentVersionFormatenforces thatagentSDKversions do not contain'-'(to protect worker name conversions), but this check is never applied in the Agent SDK selection path (getActiveAgentVersion,getAgentVersions) nor increateAgentSDKSymlinks. In contrast, Node SDK selection callscheckNoNameContainsonVersionList. This inconsistency is a contract parity issue: invalid Agent SDK version strings would be accepted and used, potentially breaking downstream worker naming or directory resolution, while Node SDK versions are validated. [ Low confidence ]